Conversation
|
|
||
| # GET /:shortened_url | ||
| def redirect | ||
| @url = Url.where(shortened_url: params[:shortened_url]).take |
There was a problem hiding this comment.
take with no argument seems very out of place to me. I think most Rails devs would expect it's alias, first.
find_by simplifies this a bit more though.
| # GET /:shortened_url | ||
| def redirect | ||
| @url = Url.where(shortened_url: params[:shortened_url]).take | ||
| if @url != nil |
There was a problem hiding this comment.
| @url.update_attribute(:click_count, @url.click_count + 1) | ||
| redirect_to @url.original_url | ||
| else | ||
| redirect_to root_path, notice: 'We couldn\'t find a link for the bitly URL you clicked.' |
There was a problem hiding this comment.
Switching to double quotes is preferable over escaping the embedded single quote.
| require 'net/http' | ||
|
|
||
| class Url < ActiveRecord::Base | ||
| ALPHANUMERIC_CHARACTERS = (('a'..'z').to_a + ('A'..'Z').to_a + ('0'..'9').to_a) |
There was a problem hiding this comment.
👍 extracting a constant for this
| end | ||
|
|
||
| def original_url_must_return_a_response | ||
| return unless original_url.present? && original_url =~ URI::regexp(['http', 'https']) |
There was a problem hiding this comment.
Matching the url against URI's regexp helper gizmo is somewhat duplicative with your check in original_url_starts_with_http_or_https. I wonder how we can remove the duplicated concept. 🤔
| errors.add(:original_url, "must respond to a HTTP request") | ||
| rescue Errno::ECONNREFUSED | ||
| errors.add(:original_url, "must respond to a HTTP request") | ||
| rescue SocketError |
There was a problem hiding this comment.
I suspect Net::HTTP is capable of raise many other errors. I'd switch to rescuing all subclasses of StandardError with bare rescue and make sure only Net::HTTP code is invoked in this method. (just the URI.parse and the get_response parts)
2 participants
@jaybobo
Implemented all features sans extra credit; will add user authentication if time allows.